Skip to content

Add documentation about OpenXR spatial entities #11015

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add documentation about OpenXR spatial entities #11015

wants to merge 1 commit into from
+1,738 −0

Conversation

BastiaanOlij
Copy link
Contributor

This PR adds documentation for the spatial entities implementation being added here: godotengine/godot#107391

@BastiaanOlij BastiaanOlij added this to the 4.x milestone Jun 11, 2025
@BastiaanOlij BastiaanOlij self-assigned this Jun 11, 2025
@BastiaanOlij BastiaanOlij added enhancement topic:xr Related to XR documentation labels Jun 11, 2025
@BastiaanOlij BastiaanOlij mentioned this pull request Jun 11, 2025
Merged
@AThousandShips AThousandShips added the waiting on pr merge PRs that can't be merged until an engine PR is merged first label Jun 11, 2025
JD-The-65th
JD-The-65th suggested changes Jun 11, 2025
View reviewed changes
Copy link

@JD-The-65th JD-The-65th left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed a few grammatical issues and typos so I wanted to make a review addressing some of them. I saw that it was also a commonality in this PR to omit commas after dependent clauses, so I left in 2 cases where I noticed it occurred, but I can go back and mark more where I noticed them.

@AThousandShips AThousandShips removed this from the 4.x milestone Jun 12, 2025
AThousandShips
AThousandShips reviewed Jun 24, 2025
View reviewed changes
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do a pass to improve sentence structure as well later

@BastiaanOlij
Copy link
Contributor Author

Thanks for the feedback everyone, just to let you all know, I'm planning on updating this PR after we're certain about the API on the implementation PR just to minimize having to go back and forth on those changes. Will be updating this PR soonish

@BastiaanOlij BastiaanOlij force-pushed the openxr_spatial_entities_docs branch 7 times, most recently from a5b0b4a to 4edb00e Compare August 6, 2025 05:56
BastiaanOlij
BastiaanOlij commented Aug 6, 2025
View reviewed changes
tutorials/xr/openxr_spatial_entities.rst
**
Q : Should we be doing update queries here to get position changes for markers??
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly a reminder to myself, I need to add this in. We now do this in the built-in logic as well.

@BastiaanOlij
Copy link
Contributor Author

I think I covered all the comments, only some outstanding stuff around parameter naming which remains a problem.

Also need to find some time to add update queries to the marker example.

AThousandShips
AThousandShips reviewed Aug 7, 2025
View reviewed changes
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions for breaking up the sentences, can look at more cases later

@BastiaanOlij BastiaanOlij force-pushed the openxr_spatial_entities_docs branch 2 times, most recently from e0bca81 to 73c533b Compare August 29, 2025 02:58
@BastiaanOlij BastiaanOlij force-pushed the openxr_spatial_entities_docs branch from 73c533b to b04e6b1 Compare September 29, 2025 09:25
@Repiteo Repiteo removed the waiting on pr merge PRs that can't be merged until an engine PR is merged first label Sep 29, 2025
@Repiteo
Copy link
Contributor

Repiteo commented Sep 29, 2025

The source PR has since been merged, so this should be prioritized once the style changes are accounted for

AThousandShips
AThousandShips approved these changes Sep 30, 2025
View reviewed changes
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it looks good now

@BastiaanOlij BastiaanOlij force-pushed the openxr_spatial_entities_docs branch 2 times, most recently from d9c1b47 to 3f59079 Compare October 1, 2025 10:50
@BastiaanOlij
Add documentation about OpenXR spatial entities
Loading
Loading status checks…
dcbf8f6
@BastiaanOlij BastiaanOlij force-pushed the openxr_spatial_entities_docs branch from 3f59079 to dcbf8f6 Compare October 1, 2025 10:53
@BastiaanOlij
Copy link
Contributor Author

Not entirely sure why its not finding some things in the class definitions because those classes should be registered. Unless I'm overlooking a typo or mistake in the references?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips

+1 more reviewer

@JD-The-65th JD-The-65th

Reviewers whose approvals may not affect merge requirements
Assignees

@BastiaanOlij BastiaanOlij

Labels
enhancement topic:xr Related to XR documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@BastiaanOlij @Repiteo @JD-The-65th @AThousandShips